Enable running most of bbin's test suite on a JVM #92

teodorlu · 2024-11-27T22:04:23Z

This PR enables a JVM-based workflow. The tests that pass without modification on a JVM can now be run on a JVM. The tests that fail on JVM only run on Babashka (for now).

In addition, test data has been updated to reflect the reality - per 2024-11-27, the latest version of org.babashka/http-server is 0.1.13.

Please answer the following questions and leave the below in as part of your PR.

This PR corresponds to an issue with a clear problem statement.
- This PR does not correspond to an issue with a clear problem statement.

This commit enables a JVM-based workflow. The tests that pass without modification on a JVM can now be run on a JVM. The tests that fail on JVM now only run on Babashka. In addition, test data has been updated to reflect the reality - per 2024-11-27, the latest version of org.babashka/http-server is 0.1.13.

teodorlu · 2024-11-27T22:07:20Z

bbin

-            [babashka.bbin.util :as util :refer [sh]]
-            [babashka.deps :as deps]
+            [babashka.bbin.util :as util :refer [sh whenbb]]
+            ;; [babashka.deps :as deps]


Commented-out lines should be deleted!

teodorlu · 2024-11-27T22:08:28Z

bbin

-           org.babashka/json {:mvn/version "0.1.1"}}})
+           ring/ring-core {:mvn/version "1.13.0"},
+           org.babashka/json {:mvn/version "0.1.1"},
+           http-kit/http-kit {:mvn/version "2.8.0"}}})


I didn't expect these dependencies to make it into ./bbin - I propose to move these deps to the :dev alias to avoid adding depdendencies to bbin users.

teodorlu · 2024-11-27T22:08:37Z

.gitignore

@@ -27,3 +27,5 @@
 /checkouts
 /classes
 /target
+
+/deps.local.edn


No trailing final newline is ugly!

teodorlu · 2024-11-27T22:09:04Z

bbin

+(defmacro ifbb [then else]
+  (if (System/getProperty "babashka.version")
+    then
+    else))


ifbb is not in use, and should be deleted.

teodorlu · 2024-11-27T22:10:08Z

dev/babashka/bbin/dev.clj

-            [clojure.core.async :refer [<!] :as async]
+            [clojure.core.async :as async :refer [<!]]


My editor did this without me noticing, I'll revert it!

teodorlu · 2024-11-27T22:10:22Z

src/babashka/bbin/scripts/common.clj

-            [babashka.bbin.util :as util :refer [sh]]
-            [babashka.deps :as deps]
+            [babashka.bbin.util :as util :refer [sh whenbb]]
+            ;; [babashka.deps :as deps]


Delete out-commented line.

teodorlu · 2024-11-27T22:10:38Z

src/babashka/bbin/util.clj

+(defmacro ifbb [then else]
+  (if (System/getProperty "babashka.version")
+    then
+    else))


ifbb is not used, should be deleted.

teodorlu · 2024-11-27T22:11:12Z

test/babashka/bbin/scripts/maven_jar_test.clj

 (def upgraded-lib
-  (assoc-in maven-lib [:coords :mvn/version] "0.1.12"))
+  (assoc-in maven-lib [:coords :mvn/version] "0.1.13"))


This seems to be a failing test on master too - 0.1.13 is the latest org.babashka/http-server version.

teodorlu · 2024-11-27T22:14:05Z

In addition to the comments in the diff, I added Kaocha and Launchpad. I did that mostly out of habit, I like using the tools.

Thoughts? Do you prefer staying "leaner" with regards to tooling?

I added Launchpad to bb.edn. If that will cause Launchpad to be added to bbin user machines, it's behavior we maybe don't want.

teodorlu · 2024-11-27T22:19:52Z

From @borkdude on Slack:

can we get rid of those bin/ scripts and add those commands to bb tasks and swap koacha for the cognitect test runner, unless you're very attached to koacha? (edited)

https://clojurians.slack.com/archives/C02FBBU61A9/p1732745453495049?thread_ts=1732744901.186879&cid=C02FBBU61A9

I generally prefer to keep maps like this sorted, I find things more easily then. If there was some previous structure here that I didn't pick up, I'll revert.

Kaocha has become heavily ingrained into my workflow through https://github.com/magnars/kaocha-runner.el. kaocha-runner requires that kaocha is on the classpath in order to work. Though that doesn't require that I put Kaocha into every project I work with. Clojure 1.12's add-lib to the rescue!

The way I'd added Launchpad in a previous commit in this PR would be bad for all bbin users: they would all be forced to download Launchpad. I don't want that. Launchpad is a development helper, it shouldn't carry weight that influences bbin users badly. So I'm removing Launchpad. We could possibly get around shipping Launchpad to bbin users by loading launchpad dynamically in the `bb launchpad` task, but that's not worth it for me for now.

teodorlu · 2025-01-03T12:55:59Z

I've removed Kaocha and Launchpad.

I like using Kaocha from within my Emacs, but I can load Kaocha with clojure.repl.deps/add-lib anyway — no reason to have two test runners (and possible confusion) in this project because of that. We do want the cognitect test runner to run tests on Babashka anyway.

As for Launchpad: Adding a Launchpad dep to bb.edn would cause everyone using bbin to have to download Launchpad, and I don't want that.

I still have to set up a deps.edn alias to run the tests with cognitect-test-runner on the JVM, and ensure it runs smoothly.

We didn't end up adding either Launchpad or Kaocha, so we might as well leave bb.edn unchanged.

(It's only used for testing)

teodorlu · 2025-01-03T17:44:25Z

@borkdude I think we're mostly there, but I'd like your thoughts on the tests that are failing in CI: https://github.com/teodorlu/bbin/actions/runs/12601558300/job/35122890533

My observations:

I run local tests on bb (bb test) and on JVM (bb test:clj), all tests are green.
I think I'm doing the same thing in CI, but in CI some of the tests are red on JVM.

It would also be helpful to know you get the CI behavior locally (red) or as I do (green).

teodorlu marked this pull request as draft November 27, 2024 22:05

teodorlu commented Nov 27, 2024

View reviewed changes

.gitignore Outdated

@@ -27,3 +27,5 @@

/checkouts

/classes

/target

/deps.local.edn

Copy link

Contributor Author

teodorlu Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No trailing final newline is ugly!

teodorlu commented Nov 27, 2024

View reviewed changes

teodorlu marked this pull request as ready for review November 27, 2024 22:14

teodorlu marked this pull request as draft November 27, 2024 22:19

teodorlu added 4 commits January 3, 2025 13:30

Sort babashka tasks alphabetically

1b3f849

I generally prefer to keep maps like this sorted, I find things more easily then. If there was some previous structure here that I didn't pick up, I'll revert.

Use bb task instead of bin/ script for Launchpad

10392b3

teodorlu changed the title ~~Enable running (most of) bbin's test suite on a JVM~~ Enable running most of bbin's test suite on a JVM Jan 3, 2025

teodorlu added 10 commits January 3, 2025 17:14

Undo reordering of tasks in bb.edn

c7664c7

We didn't end up adding either Launchpad or Kaocha, so we might as well leave bb.edn unchanged.

Clean up after removing Launchpad

5ea4d80

Remove unused code

9a02989

Run JVM tests with bb test:clj

ffce005

Remove Kaocha (again)

3deff00

Avoid shipping ring/ring-core to bbin users

b92def3

(It's only used for testing)

Delete commented-out code

212794e

bb test:clj: allow passing command line arguments

f3f0ddc

Add test deps to :dev alias

0ec000d

Undo accidential code formatting

c5a6eb4

teodorlu added 5 commits January 3, 2025 18:22

upgrade-maven-jar-test runs on JVM after all!

6974553

invalid-bin-config-test also runs on clj

1f170e1

Remove unused import

d2298da

Remove unused import

01335e4

Run clj tests on CI too

1181287

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable running most of bbin's test suite on a JVM #92

Enable running most of bbin's test suite on a JVM #92

teodorlu commented Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu Nov 27, 2024

teodorlu commented Nov 27, 2024

teodorlu commented Nov 27, 2024 •

edited

Loading

teodorlu commented Jan 3, 2025 •

edited

Loading

teodorlu commented Jan 3, 2025

		[clojure.core.async :refer [<!] :as async]
		[clojure.core.async :as async :refer [<!]]

Enable running most of bbin's test suite on a JVM #92

Are you sure you want to change the base?

Enable running most of bbin's test suite on a JVM #92

Conversation

teodorlu commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodorlu commented Nov 27, 2024

teodorlu commented Nov 27, 2024 • edited Loading

teodorlu commented Jan 3, 2025 • edited Loading

teodorlu commented Jan 3, 2025

teodorlu commented Nov 27, 2024 •

edited

Loading

teodorlu commented Jan 3, 2025 •

edited

Loading